Skip to content

[SEARCHEXP-1358]Handle relative path and resolve to their real path#164

Merged
mungodewar merged 7 commits into
Skyscanner:forkfrom
gc-skyscanner:handle_relative_path
Jul 11, 2023
Merged

[SEARCHEXP-1358]Handle relative path and resolve to their real path#164
mungodewar merged 7 commits into
Skyscanner:forkfrom
gc-skyscanner:handle_relative_path

Conversation

@gc-skyscanner

Copy link
Copy Markdown

Description

Ticket: SEARCHEXP-1358
Investigation and decision log: Decision log
For the consumer side, it will fail to compile when importing a typed logic from the common package, which is imported by npm local import

We discussed the proposals listed in this decision log, and we all agree to adopt Option 3 as a medium-term option before new potential production standard carry out. That is 👇:

  • Add a logic to handle the path which is not a part of its node_modules

ℹ️ We will release a beta version first, and verify in CI that this works in other microsites. Once everything goes well, we will release a main version

How to solve it

  • Identify local paths and resolve to their real path

Screenshots

Compile and build successfully in Banana with the changes

? bpkReactScriptsConfig.babelIncludePrefixes.map(
(prefix) => new RegExp(`node_modules[\\/]${prefix}`)
)
? bpkReactScriptsConfig.babelIncludePrefixes.map(prefix => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we able to add unit tests for this at all?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be a good idea, added.
but seems BRS is lacking unit tests and there is no process in the pipeline to run the unit test automatically(I couldn’t find it if I don’t miss something), in this case, seems adding unit tests for this change doesn’t make much sense. Seems the test facility in BRS is not fully equipped, what do you think 🤔️

Comment thread packages/react-scripts/package.json Outdated
{
"name": "@skyscanner/backpack-react-scripts",
"version": "10.4.0",
"version": "10.4.1",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should happen after PR is merged and new version is released.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know, thanks!

@arnaugm

Copy link
Copy Markdown

beta available in https://www.npmjs.com/package/@skyscanner/backpack-react-scripts/v/10.4.1-beta.1

@mungodewar mungodewar merged commit 5610d07 into Skyscanner:fork Jul 11, 2023
mungodewar pushed a commit that referenced this pull request Jul 11, 2023
…164)

* handle relative path

* remove console.log

* handle relative path

* unformat

* update version

* add unit test

* revert change log

---------

Co-authored-by: gc.zhu <gc.zhu@skyscanner.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants